Skip to content

ext/dom: php_dom_free_templated_content() use-after-free. - #23341

Open
devnexen wants to merge 3 commits into
php:PHP-8.4from
devnexen:gh23334
Open

devnexen wants to merge 3 commits into
php:PHP-8.4from
devnexen:gh23334

Conversation

@devnexen

@devnexen devnexen commented Aug 18, 2026 •

Copy link
Copy Markdown
Member

Fix #23334

The fragment was freed with xmlFreeNode(), assuming no userland reference into template contents could exist. getElementById() reaches those nodes, so freeing the template element left dangling proxies.

The children now go through php_libxml_node_free_list(), and the fragment is freed only when it has no proxy; otherwise its parent link is cleared so it is freed with the last reference.

@devnexen

Copy link
Copy Markdown
Member Author

There can be two more changes (e.g. Dom\HTMLTemplateElement) but they are more master material. @ndossche let me know if even the 2nd commit is too.

@devnexen
devnexen marked this pull request as ready for review August 18, 2026 07:37
@Sjord

Sjord commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Looks good to me.

I am not sure this totally fixes #23334.

@ndossche

Copy link
Copy Markdown
Member

Looks good to me.

I am not sure this totally fixes #23334.

Well this doesn't add the template API, so not really.

I'll have a deep look at this PR in the weekend.

@ndossche
ndossche self-requested a review October 13, 2026 19:00
@ndossche

ndossche commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

This doesn't look right. getElementById shouldn't reach the template element in the first place.
This patch instead should do the job to prevent the UAF:

diff --git a/ext/dom/html5_parser.c b/ext/dom/html5_parser.c
index 24f89cb0faf..fa68004dc4d 100644
--- a/ext/dom/html5_parser.c
+++ b/ext/dom/html5_parser.c
@@ -39,6 +39,7 @@ typedef struct work_list_item {
     uintptr_t current_active_namespace;
     xmlNodePtr lxml_parent;
     xmlNsPtr lxml_ns;
+    bool in_template;
 } work_list_item;
 
 static void lexbor_libxml2_bridge_work_list_item_push(
@@ -46,7 +47,8 @@ static void lexbor_libxml2_bridge_work_list_item_push(
     lxb_dom_node_t *node,
     uintptr_t current_active_namespace,
     xmlNodePtr lxml_parent,
-    xmlNsPtr lxml_ns
+    xmlNsPtr lxml_ns,
+    bool in_template
 )
 {
     work_list_item *item = (work_list_item *) lexbor_array_obj_push_wo_cls(array);
@@ -54,6 +56,7 @@ static void lexbor_libxml2_bridge_work_list_item_push(
     item->current_active_namespace = current_active_namespace;
     item->lxml_parent = lxml_parent;
     item->lxml_ns = lxml_ns;
+    item->in_template = in_template;
 }
 
 static unsigned short sanitize_line_nr(size_t line)
@@ -124,13 +127,14 @@ static lexbor_libxml2_bridge_status lexbor_libxml2_bridge_convert(
     lexbor_array_obj_init(&work_list, WORK_LIST_INIT_SIZE, sizeof(work_list_item));
 
     for (lxb_dom_node_t *node = start_node; node != NULL; node = node->prev) {
-        lexbor_libxml2_bridge_work_list_item_push(&work_list, node, LXB_NS__UNDEF, root, NULL);
+        lexbor_libxml2_bridge_work_list_item_push(&work_list, node, LXB_NS__UNDEF, root, NULL, false);
     }
 
     work_list_item *current_stack_item;
     while ((current_stack_item = lexbor_array_obj_pop(&work_list)) != NULL) {
         lxb_dom_node_t *node = current_stack_item->node;
         xmlNodePtr lxml_parent = current_stack_item->lxml_parent;
+        bool in_template = current_stack_item->in_template;
 
         /* CDATA section and processing instructions don't occur in parsed HTML documents.
          * The historical types are not emitted by the parser either. */
@@ -173,7 +177,9 @@ static lexbor_libxml2_bridge_status lexbor_libxml2_bridge_convert(
 			 * Other types of nodes contain their children directly. */
 			xmlNodePtr lxml_child_parent = lxml_element;
 			lxb_dom_node_t *child_node = element->node.last_child;
+			bool child_in_template = in_template;
 			if (lxb_html_tree_node_is(&element->node, LXB_TAG_TEMPLATE)) {
+				child_in_template = true;
 				if (create_default_ns) {
 					lxml_child_parent = xmlNewDocFragment(lxml_doc);
 					if (UNEXPECTED(lxml_child_parent == NULL)) {
@@ -198,7 +204,8 @@ static lexbor_libxml2_bridge_status lexbor_libxml2_bridge_convert(
                     child_node,
                     entering_namespace,
                     lxml_child_parent,
-                    current_lxml_ns
+                    current_lxml_ns,
+                    child_in_template
                 );
             }
 
@@ -275,7 +282,9 @@ static lexbor_libxml2_bridge_status lexbor_libxml2_bridge_convert(
 
                 /* xmlIsID does some other stuff too that is irrelevant here. */
                 if (local_name_length == 2 && local_name[0] == 'i' && local_name[1] == 'd' && lxml_attr->ns == NULL) {
-                    if (xmlAddID(NULL, lxml_doc, value, lxml_attr) == 0) {
+                    /* Elements inside template contents are not in the document tree, so they must not be registered
+                     * in the ID cache. They still get marked as an ID attribute for when they are moved into the tree. */
+                    if (in_template || xmlAddID(NULL, lxml_doc, value, lxml_attr) == 0) {
                         /* If the ID already exists, the ID attribute still needs to be marked as an ID. */
                         lxml_attr->atype = XML_ATTRIBUTE_ID;
                     }

Having a proper $content field that will allow the access is a separate feature request, then we need to fix the proxies but in a cleaner way rather than plumbing various different places.

Fix php#23334

The fragment was freed with xmlFreeNode(), assuming no userland
reference into template contents could exist. getElementById() reaches
those nodes, so freeing the template element left dangling proxies.

The children now go through php_libxml_node_free_list(), and the
fragment is freed only when it has no proxy; otherwise its parent link
is cleared so it is freed with the last reference.
Upward walks followed the fragment's parent link to its host, placing
template contents in the host document instead of rooting them at the
fragment. They now stop at a fragment via php_dom_parent_node(), except
the cycle check which the spec resolves against host-including ancestors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants